Add build_target parameter to PythonPackage#3575
Add build_target parameter to PythonPackage#3575Flamefire wants to merge 3 commits intoeasybuilders:developfrom
build_target parameter to PythonPackage#3575Conversation
| @@ -408,17 +408,18 @@ def extra_options(extra_vars=None): | |||
| extra_vars = {} | |||
| extra_vars.update({ | |||
| 'buildcmd': [None, "Command for building the package (e.g. for custom builds resulting in a whl file). " | |||
There was a problem hiding this comment.
Please indicate that this is deprecated, e.g.
| 'buildcmd': [None, "Command for building the package (e.g. for custom builds resulting in a whl file). " | |
| 'buildcmd': [None, "DEPRECATED: Command for building the package (e.g. for custom builds resulting in a whl file). " |
There was a problem hiding this comment.
This is not deprecated. It is exactly what it states: If set it will be used for building the package.
There is a deprecated (and undocumented) code path that it uses setup.py {buildcmd} when buildcmd starts with build or build_ext to avoid breaking existing code.
There was a problem hiding this comment.
Does it make sense to keep both buildcmd and build_target supported though?
buildcmd clearly leaves room for confusion, so I'm also in favor of deprecating it, and using build_target instead. That should work, no?
| if not build_cmd: | ||
| build_cmd = 'build' # Default value for setup.py | ||
| build_cmd = f"{self.python_cmd} setup.py {build_cmd}" | ||
| build_cmd = f"{self.python_cmd} setup.py {self.cfg['build_target']}" | ||
| elif any(build_cmd.startswith(cmd) for cmd in ('build ', 'build_ext ')) or re.match(r'\w+', build_cmd): | ||
| if ' ' not in build_cmd: | ||
| self.log.deprecated("Use 'build_target' instead of 'buildcmd' " | ||
| "to pass the build target to setup.py", '5.1') | ||
| else: | ||
| self.log.deprecated("Use 'build_target' and 'buildopts' instead of 'buildcmd' " | ||
| "to pass arguments to setup.py", '5.1') | ||
| build_cmd = f"{self.python_cmd} setup.py {build_cmd}" |
There was a problem hiding this comment.
I would like to see a more direct discouragement of using buildcmd, now it would slip by without your knowing it was deprecated depending on what you set it to.
grepping through the easyconfigs, i think only PyTorch was treating buildcmd this was, rest was already treating it as build_target, so.. lets just fix PyTorch and forget about that silly case?
if self.cfg['buildcmd'] is not None: # should never be set anymore
build_target = buildcmd
self.log.deprecated('use build target and buildopts instead of buildcmd blablabla')
else:
build_target = buildcmd
...
build_cmd = f'{self.python_cmd} setup.py {build_target}"having it marked clearly deprecated, and the build_target being more clear in what it should contain, I think this is safe enough to change?
You can include a warning on not adding build_opts in there is we wanted, but i'm not sure we need to.
There was a problem hiding this comment.
The idea here was to allow using a build command, not removing that. Imagine some Python package that requires ./build_python.sh to create a wheel. See the documentation of the option.
|
Current behavior:
So for 1. it works as intended, only 2 is basically broken/unintuitive. The intention here is:
The We could have easyblocks implement Maybe: Add |
|
@Flamefire I changed to target branch in this PR from |
Using `buildcmd` to pass options to `setup.py` is confusing and makes it impossible to fully replace the build command. Introduce `build_target` similar to `install_target` and deprecate passing a target and optional options via `buildcmd`. The old behavior is used when `buildcmd` is a known setup.py command such as `build` or `build_ext` or a single word, e.g. `clean`.
Flamefire
force-pushed
the
pythonpackage-build-setuppy
branch
from
aa56a6b to
ceefb81
Compare
3 participants
Using
buildcmdto pass options tosetup.pyis confusing and makes it impossible to fully replace the build command.Introduce
build_targetsimilar toinstall_targetand deprecate passing a target and optional options viabuildcmd.The old behavior is used when
buildcmdis a known setup.py command such asbuildorbuild_extor a single word, e.g.clean.My first attempt was to only detect
buildcmd = '%(python)s ...'as an immediate fix for #3570 but that would limit us to much going forward when a Python package build uses a custom command like./build_wheel --fooIf this is acceptable I can open an accompanying PR to update the EasyConfigs to replace
buildcmd.